-
Couldn't load subscription status.
- Fork 3.4k
Don't build MAIN_MODULE as RELOCATABLE #25522
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
cd1b8ce to
e8f95c7
Compare
Also, add an assertion to dynCall. Split out from emscripten-core#25522.
Also, add an assertion to dynCall. Split out from emscripten-core#25522.
8c32591 to
d40087a
Compare
|
I still need to get some stats on the benefits of this, but that tests are all now passing so I think this should be good to review now. An open question is: What do we do about |
Also, add an assertion to dynCall. Split out from emscripten-core#25522.
d40087a to
6008112
Compare
|
Yeah, I see what you mean, removing RELOCATABLE first would simplify this a little. I don't have a preference though. I do see some tests still fail here though? |
Also, add an assertion to dynCall. Split out from #25522.
637ed91 to
f56f631
Compare
0294236 to
d499627
Compare
|
Ok, aside from the problem of what to do with The stats look really promising. See the new PR description for the great improvements measured when building a dylink version of binaryen. |
d499627 to
2c6de78
Compare
| #endif | ||
| for (var [symName, entry] of Object.entries(GOT)) { | ||
| if (entry.value == 0) { | ||
| if (entry.value == {{{ to64(-1) }}}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe comment what this means? (tombstone?)
| return 0; | ||
| } | ||
| ''', | ||
| 'a: loaded\na: b (prev: (null))\na: c (prev: b)\n', cflags=extra_args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this test?
23d619c to
a2e97ec
Compare
The normal dependency system can handle including these when needed.
The main advantage here is that main module no longer requires relocation entries for symbols defined locally. To show the benefits of this approach I build binaryan_wasm with `-sMAIN_MODULE=1` both before and after this change. Before: Wasm Size: 22.6M __wasm_apply_data_relocs size: 123k After: Wasm Size: 16.6M __wasm_apply_data_relocs size: 0 (no longer exists) Fixes: emscripten-core#12682
a2e97ec to
4425035
Compare
|
I solves the |
The main advantage here is that main module no longer requires
relocation entries for symbols defined locally.
To show the benefits of this approach I build binaryan_wasm
with
-sMAIN_MODULE=1both before and after this change.Before:
Wasm Size: 22.6M
__wasm_apply_data_relocs size: 123k
After:
Wasm Size: 16.6M
__wasm_apply_data_relocs size: 0 (no longer exists)
Since this is quite a major change, I've kept the
-sRELOCATABLEoption available so folks that use this to get the old behaviour when
linking their main module. In order to tests this path too now run
all the
@needs_dylinktests in test_core.py with and without-sRELOCTABLE. Hopefully we can remove this configurationcompletely at some point in the future.
Fixes: #12682